From f730deff0603b44831e40b51aa054823126fcffc Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 1 Feb 2016 13:30:43 -0800 Subject: [PATCH] Refactor URL parsing, be more robust for decoding errors URL parsing now returns a `CargoResult` and also change a few `unwrap()` calls to returning a `CargoResult` when decoding various bits and pieces of information. --- src/bin/git_checkout.rs | 12 +++--------- src/bin/install.rs | 4 ++-- src/cargo/core/package_id.rs | 12 +++++++++--- src/cargo/core/resolver/encode.rs | 16 +++++++++++----- src/cargo/core/source.rs | 28 +++++++++++++++------------- src/cargo/ops/registry.rs | 2 +- src/cargo/sources/git/utils.rs | 4 ++-- src/cargo/sources/registry.rs | 2 +- src/cargo/util/to_url.rs | 15 +++++++++------ src/cargo/util/toml.rs | 2 +- 10 files changed, 54 insertions(+), 43 deletions(-) diff --git a/src/bin/git_checkout.rs b/src/bin/git_checkout.rs index e67844cce..f7762483e 100644 --- a/src/bin/git_checkout.rs +++ b/src/bin/git_checkout.rs @@ -1,6 +1,6 @@ use cargo::core::source::{Source, SourceId, GitReference}; use cargo::sources::git::{GitSource}; -use cargo::util::{Config, CliResult, CliError, human, ToUrl}; +use cargo::util::{Config, CliResult, ToUrl}; #[derive(RustcDecodable)] pub struct Options { @@ -37,20 +37,14 @@ pub fn execute(options: Options, config: &Config) -> CliResult> { options.flag_locked)); let Options { flag_url: url, flag_reference: reference, .. } = options; - let url = try!(url.to_url().map_err(|e| { - human(format!("The URL `{}` you passed was \ - not a valid URL: {}", url, e)) - }) - .map_err(|e| CliError::new(e, 1))); + let url = try!(url.to_url()); let reference = GitReference::Branch(reference.clone()); let source_id = SourceId::for_git(&url, reference); let mut source = GitSource::new(&source_id, config); - try!(source.update().map_err(|e| { - CliError::new(human(format!("Couldn't update {:?}: {:?}", source, e)), 1) - })); + try!(source.update()); Ok(None) } diff --git a/src/bin/install.rs b/src/bin/install.rs index 78d8f58fd..e7d3d03f7 100644 --- a/src/bin/install.rs +++ b/src/bin/install.rs @@ -1,6 +1,6 @@ use cargo::ops; use cargo::core::{SourceId, GitReference}; -use cargo::util::{CliResult, Config, ToUrl, human}; +use cargo::util::{CliResult, Config, ToUrl}; #[derive(RustcDecodable)] pub struct Options { @@ -116,7 +116,7 @@ pub fn execute(options: Options, config: &Config) -> CliResult> { }; let source = if let Some(url) = options.flag_git { - let url = try!(url.to_url().map_err(human)); + let url = try!(url.to_url()); let gitref = if let Some(branch) = options.flag_branch { GitReference::Branch(branch) } else if let Some(tag) = options.flag_tag { diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs index bd3a66d94..dfba5d7c6 100644 --- a/src/cargo/core/package_id.rs +++ b/src/cargo/core/package_id.rs @@ -38,13 +38,19 @@ impl Decodable for PackageId { fn decode(d: &mut D) -> Result { let string: String = try!(Decodable::decode(d)); let regex = Regex::new(r"^([^ ]+) ([^ ]+) \(([^\)]+)\)$").unwrap(); - let captures = regex.captures(&string).expect("invalid serialized PackageId"); + let captures = try!(regex.captures(&string).ok_or_else(|| { + d.error("invalid serialized PackageId") + })); let name = captures.at(1).unwrap(); let version = captures.at(2).unwrap(); let url = captures.at(3).unwrap(); - let version = semver::Version::parse(version).ok().expect("invalid version"); - let source_id = SourceId::from_url(url); + let version = try!(semver::Version::parse(version).map_err(|_| { + d.error("invalid version") + })); + let source_id = try!(SourceId::from_url(url).map_err(|e| { + d.error(&e.to_string()) + })); Ok(PackageId { inner: Arc::new(PackageIdInner { diff --git a/src/cargo/core/resolver/encode.rs b/src/cargo/core/resolver/encode.rs index 256e7a49c..31bc22b7a 100644 --- a/src/cargo/core/resolver/encode.rs +++ b/src/cargo/core/resolver/encode.rs @@ -182,15 +182,21 @@ impl Decodable for EncodablePackageId { fn decode(d: &mut D) -> Result { let string: String = try!(Decodable::decode(d)); let regex = Regex::new(r"^([^ ]+) ([^ ]+)(?: \(([^\)]+)\))?$").unwrap(); - let captures = regex.captures(&string) - .expect("invalid serialized PackageId"); + let captures = try!(regex.captures(&string).ok_or_else(|| { + d.error("invalid serialized PackageId") + })); let name = captures.at(1).unwrap(); let version = captures.at(2).unwrap(); - let source = captures.at(3); - - let source_id = source.map(|s| SourceId::from_url(s)); + let source_id = match captures.at(3) { + Some(s) => { + Some(try!(SourceId::from_url(s).map_err(|e| { + d.error(&e.to_string()) + }))) + } + None => None, + }; Ok(EncodablePackageId { name: name.to_string(), diff --git a/src/cargo/core/source.rs b/src/cargo/core/source.rs index c32e7227f..83098c575 100644 --- a/src/cargo/core/source.rs +++ b/src/cargo/core/source.rs @@ -91,14 +91,14 @@ impl SourceId { /// libssh2-static-sys#80e71a3021618eb05\ /// 656c58fb7c5ef5f12bc747f"); /// ``` - pub fn from_url(string: &str) -> SourceId { + pub fn from_url(string: &str) -> CargoResult { let mut parts = string.splitn(2, '+'); let kind = parts.next().unwrap(); let url = parts.next().unwrap(); match kind { "git" => { - let mut url = url.to_url().unwrap(); + let mut url = try!(url.to_url()); let mut reference = GitReference::Branch("master".to_string()); for (k, v) in url.query_pairs() { match &k[..] { @@ -114,18 +114,18 @@ impl SourceId { let precise = url.fragment().map(|s| s.to_owned()); url.set_fragment(None); url.set_query(None); - SourceId::for_git(&url, reference).with_precise(precise) - } + Ok(SourceId::for_git(&url, reference).with_precise(precise)) + }, "registry" => { - let url = url.to_url().unwrap(); - SourceId::new(Kind::Registry, url) - .with_precise(Some("locked".to_string())) + let url = try!(url.to_url()); + Ok(SourceId::new(Kind::Registry, url) + .with_precise(Some("locked".to_string()))) } "path" => { - let url = url.to_url().unwrap(); - SourceId::new(Kind::Path, url) + let url = try!(url.to_url()); + Ok(SourceId::new(Kind::Path, url)) } - _ => panic!("Unsupported serialized SourceId"), + kind => Err(human(format!("unsupported source protocol: {}", kind))) } } @@ -155,7 +155,7 @@ impl SourceId { // Pass absolute path pub fn for_path(path: &Path) -> CargoResult { - let url = try!(path.to_url().map_err(human)); + let url = try!(path.to_url()); Ok(SourceId::new(Kind::Path, url)) } @@ -267,8 +267,10 @@ impl Encodable for SourceId { impl Decodable for SourceId { fn decode(d: &mut D) -> Result { - let string: String = Decodable::decode(d).ok().expect("Invalid encoded SourceId"); - Ok(SourceId::from_url(&string)) + let string: String = try!(Decodable::decode(d)); + SourceId::from_url(&string).map_err(|e| { + d.error(&e.to_string()) + }) } } diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 292b0729e..e47b0f7a2 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -167,7 +167,7 @@ pub fn registry(config: &Config, } = try!(registry_configuration(config)); let token = token.or(token_config); let index = index.or(index_config).unwrap_or(RegistrySource::default_url()); - let index = try!(index.to_url().map_err(human)); + let index = try!(index.to_url()); let sid = SourceId::for_registry(&index); let api_host = { let mut src = RegistrySource::new(&sid, config); diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index 1149bfffd..f7dc89d7f 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -257,7 +257,7 @@ impl<'a> GitCheckout<'a> { })); } - let url = try!(source.to_url().map_err(human)); + let url = try!(source.to_url()); let url = url.to_string(); let repo = try!(git2::Repository::clone(&url, into).chain_error(|| { internal(format!("failed to clone {} into {}", source.display(), @@ -278,7 +278,7 @@ impl<'a> GitCheckout<'a> { fn fetch(&self, cargo_config: &Config) -> CargoResult<()> { info!("fetch {}", self.repo.path().display()); - let url = try!(self.database.path.to_url().map_err(human)); + let url = try!(self.database.path.to_url()); let url = url.to_string(); let refspec = "refs/heads/*:refs/heads/*"; try!(fetch(&self.repo, &url, refspec, &cargo_config)); diff --git a/src/cargo/sources/registry.rs b/src/cargo/sources/registry.rs index f89f6bae7..0b40e17c5 100644 --- a/src/cargo/sources/registry.rs +++ b/src/cargo/sources/registry.rs @@ -254,7 +254,7 @@ impl<'cfg> RegistrySource<'cfg> { pub fn url(config: &Config) -> CargoResult { let config = try!(ops::registry_configuration(config)); let url = config.index.unwrap_or(DEFAULT.to_string()); - url.to_url().map_err(human) + url.to_url() } /// Get the default url for the registry diff --git a/src/cargo/util/to_url.rs b/src/cargo/util/to_url.rs index c86857082..c250937b3 100644 --- a/src/cargo/util/to_url.rs +++ b/src/cargo/util/to_url.rs @@ -1,22 +1,25 @@ -use url::Url; use std::path::Path; +use url::Url; + +use util::{human, CargoResult}; + pub trait ToUrl { - fn to_url(self) -> Result; + fn to_url(self) -> CargoResult; } impl<'a> ToUrl for &'a str { - fn to_url(self) -> Result { + fn to_url(self) -> CargoResult { Url::parse(self).map_err(|s| { - format!("invalid url `{}`: {}", self, s) + human(format!("invalid url `{}`: {}", self, s)) }) } } impl<'a> ToUrl for &'a Path { - fn to_url(self) -> Result { + fn to_url(self) -> CargoResult { Url::from_file_path(self).map_err(|()| { - format!("invalid path url `{}`", self.display()) + human(format!("invalid path url `{}`", self.display())) }) } } diff --git a/src/cargo/util/toml.rs b/src/cargo/util/toml.rs index 7de06ddc2..cdb3d999c 100644 --- a/src/cargo/util/toml.rs +++ b/src/cargo/util/toml.rs @@ -817,7 +817,7 @@ impl TomlDependency { .or_else(|| details.tag.clone().map(GitReference::Tag)) .or_else(|| details.rev.clone().map(GitReference::Rev)) .unwrap_or_else(|| GitReference::Branch("master".to_string())); - let loc = try!(git.to_url().map_err(human)); + let loc = try!(git.to_url()); SourceId::for_git(&loc, reference) }, (None, Some(path)) => { -- 2.30.2